-
Notifications
You must be signed in to change notification settings - Fork 220
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
ci: Test against various kernels on PR #373
Conversation
~~ New workflow requires approval for running. ~~ I ran them on my personal fork jschwinger233#1, it looks code logic is fine but |
9171095
to
ddefc17
Compare
Alright, looks better: https://github.com/jschwinger233/dae/actions/runs/7262360947/job/19785369670?pr=1 |
Hey @jschwinger233, thanks for the contribution. As far as I understood, are these proposed changes mainly for |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧪 Since the PR has been fully tested, please consider merging it.
That's right. Although not all cases are covered (only TCP + dip filter + socks5 are tested), this workflow gives a good sense of whether dae works under different kernels. If bpf verifier complains due to complexity issues, this workflow definitely will catch them. We can also extend test suite as a follow-up if we want. |
.github/workflows/test.yml
Outdated
push: | ||
branches: [ main ] | ||
pull_request: | ||
branches: [ main ] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
branches: [ main ] | |
paths: | |
- "**/*.go" | |
- "**/*.c" | |
- "**/*.h" | |
- "go.mod" | |
- "go.sum" | |
- ".github/workflows/test.yml" |
Add trigger conditions (based on files).
.github/workflows/test.yml
Outdated
push: | ||
branches: [ main ] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
push: | |
branches: [ main ] |
For trouble shooting purpose, we may do the kernel-related testing in the PR level to ensure we fix the incompatible issue before we merge it to main branch.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jschwinger233 Thanks for the fabulous work. One little comment/suggestion from my end, would you like to rename test.yml
to kernel-test.yml
to clarify the purpose of the test?
ddefc17
to
7f7737a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Once again appreciate all the efforts. |
Closes: #368